Skip to content

Conversation

scottmcm
Copy link
Member

This way comparing [NonZeroU8; 8] is just as fast as comparing [u8; 8].

This way comparing `[NonZeroU8; 8]` is just as fast as comparing `[u8; 8]`.
@rust-highfive
Copy link
Contributor

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@yaahc
Copy link
Member

yaahc commented Dec 11, 2021

Very fancy, particularly appreciate the comments.

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 11, 2021

📌 Commit 24affba has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2021
@the8472
Copy link
Member

the8472 commented Dec 11, 2021

Since it's a specialization trait it should be possible to extend this to nested arrays/slices of arrays.

@matthiaskrgr
Copy link
Member

@bors rollup=never might affect performance

@bors
Copy link
Collaborator

bors commented Dec 14, 2021

⌛ Testing commit 24affba with merge 83b32f2...

@bors
Copy link
Collaborator

bors commented Dec 14, 2021

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 83b32f2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2021
@bors bors merged commit 83b32f2 into rust-lang:master Dec 14, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (83b32f2): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -5.2% on incr-unchanged builds of inflate)
  • Small regression in instruction counts (up to 1.4% on incr-unchanged builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Dec 14, 2021
@scottmcm scottmcm deleted the more-array-raw-eq branch December 15, 2021 05:02
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2021
…t-slices, r=dtolnay

Do array-slice equality via array equality, rather than always via slices

~~Draft because it needs a rebase after rust-lang#91766 eventually gets through bors.~~

This enables the optimizations from rust-lang#85828 to be used for array-to-slice comparisons too, not just array-to-array.

For example, <https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=5f9ba69b3d5825a782f897c830d3a6aa>
```rust
pub fn demo(x: &[u8], y: [u8; 4]) -> bool {
    *x == y
}
```
Currently writes the array to stack for no reason:
```nasm
	sub	rsp, 4
	mov	dword ptr [rsp], edx
	cmp	rsi, 4
	jne	.LBB0_1
	mov	eax, dword ptr [rdi]
	cmp	eax, dword ptr [rsp]
	sete	al
	add	rsp, 4
	ret

.LBB0_1:
	xor	eax, eax
	add	rsp, 4
	ret
```
Whereas with the change in this PR it just compares it directly:
```nasm
	cmp	rsi, 4
	jne	.LBB1_1
	cmp	dword ptr [rdi], edx
	sete	al
	ret

.LBB1_1:
	xor	eax, eax
	ret
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants